Add vertical mixing coefficients module#310
Add vertical mixing coefficients module#310mark-petersen merged 19 commits intoE3SM-Project:developfrom
Conversation
katsmith133
requested review from
alicebarthel,
mwarusz,
philipwjones and
vanroekel
philipwjones
left a comment
philipwjones
left a comment
There was a problem hiding this comment.
This fails on Frontier GPU with a memory access error, so some array/variable may be missing or not scoped? I have some other comments spread below.
@philipwjones did the error provide any more information than that? At the moment my Frontier build is stopping at the cmake step (it keeps hanging at the |
|
@katsmith133 No additional info and I didn't have a chance to narrow it further, but I forgot to mention that it failed both EOS and VertMix unit tests, so it could point to something in the EOS additions. And I think this was with the AMD compiler - I can look into it further next week if no one gets to it first. |
Thanks @philipwjones! I am in a Deep Learning course this week, and probably will not get to it. So if you have time, that would be appreciated! If not, I can pick it back up next week. |
mwarusz
left a comment
mwarusz
left a comment
There was a problem hiding this comment.
I had a chance to look into the test failures of this PR on Frontier. There are issues with accessing host pointers on the device in EosTest.cpp and VertMixTest.cpp. My suggestion shows how to fix one loop, but similar changes need to be applied for all accesses to VCoord->ZMid and various Mesh->X arrays in these two tests.
|
Just pushed some changes to fix the frontier issue in EOS test driver (Vmix test driver mods will come soon). The commit also adds some additional EOS checks and cleans up some error handling, so folks should pull these changes if working on this PR |
mwarusz
left a comment
mwarusz
left a comment
There was a problem hiding this comment.
I have a few more comments and suggestions. There is a couple of serial vertical loops in this PR that I think could be easily done in parallel.
|
Ok, I pushed a new version of the vert mix test driver that fixes the Frontier issues, expands some error checks to full arrays, and cleans up a few things. This now passes ctests on Frontier. |
|
@philipwjones Thanks for the help in fixing the Frontier issues! @mwarusz, @cbegeman, and @vanroekel I believe I have addressed all of your comments. Please let me know if not. Thanks! |
| Real PInt = | ||
| 0.5_Real * (Pressure(ICell, K) + Pressure(ICell, K - 1)); | ||
| Real SpInt = 0.5_Real * (SpecVol(ICell, K) + SpecVol(ICell, K - 1)); | ||
| Real AlphaInt = calcAlpha(SaInt, CtInt, PInt, SpInt); |
There was a problem hiding this comment.
we may want to put alpha and beta in member arrays as KPP will need these too and then we won't have to recalculate (I think).
There was a problem hiding this comment.
I'll keep this as a note to change when we implement KPP. Thanks for pointing it out!
katsmith133
force-pushed
the
omega/add-vertical-mixing
branch
from
543fdb9 to
0dfdfdc
Compare
- added missing OMEGA_SCOPE statements - added some checks/tests to EOS driver - modified some error handling
- added needed OMEGA_SCOPE calls for some vars - update error handling - expanded error checks to full arrays - cleaned up some variable names
katsmith133
requested review from
alicebarthel
and removed request for
alicebarthel
katsmith133
force-pushed
the
omega/add-vertical-mixing
branch
from
0dfdfdc to
57fb146
Compare
|
Passes all tests on perlmutter and frontier, CPU and GPU, including Test commands:
|
|
Thank you @katsmith133 and congratulations on the completion of this project. |
This brings in: - E3SM-Project/Omega#288 - E3SM-Project/Omega#309 - E3SM-Project/Omega#314 - E3SM-Project/Omega#310 The last of these, Add vertical mixing coefficients module, is likely responsible for non-bit-for-bit changes in the `manufactured_solution/convergence_both/del4` tests.
7 participants
Add the
VertMixclass to Omega, which consists of:Methods to calculate the vertical diffusivity and viscosity using (for now just) the
PPoption, which can linearly add contributions from background, convective, and shear mixing based upon user choices in the config file. TheVertMixoptions and parameter values are set in the yaml config file. There is a dummyKPPoption in there now to set up the structure for adding it in the near future (I can remove if we think this is messy). This PR includes tests for each individual mixing process (background, convective, and shear), as well as one that linearly adds all of them together. Because both the convective and shear mixing require the Brunt Vaiasla frequency, this PR also adds the calculation ofBruntVaisalaFreqto the EOS class and adds corresponding tests. TheBruntVaisalaFreqis calculated in the way that was in MPAS-O if the Linear EOS is used (i.e. with linear coefficients and the derivative based upon changes inz) and is calculated in the same way the TEOS-10 package calculates it if the TEOS-10 EOS is used (i.e. with non-linear coefficients and the derivative based upon changes inp).Ran ctests successfully on pm-cpu, pm-gpu, and chrysalis.
Checklist